Improve Block Toolbar Semantics/Accessibility #54513
Conversation
jeryj
requested review from
ellatrix,
ajitbohra,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
|
cc @afercia |
afercia
added
the
[Focus] Accessibility (a11y)
|
Added the Accessibility label so that this gets into the radar of other contributors willing to have a look into it. Cc @alexstine |
|
Thanks @jeryj for your effort with fixing the DOM order of the toolbar and improving the intereaction. I tested a bit this PR in the Post Editor and I'd think it's moving in a good direction. Some of the changes here are necessary to fix the accessibility breakage introduced in #52123 and I'd encourage anyone to help this PR move on. There are a few things to consider though. First of all, it is necessary to preserve some affordance and always provide a consistent mechanism to move / exit the block toolbar. So far, the convention in the editor is that when editing a block:
There's a lot of different modes to take into account and I do realize such a big change is complex. So far I noticed a few things that don't work as expected. Trying to summarize: Default (top toolbar not enabled)Select mode
Edit mode
RIght now, in this PR the Shift+Tab order is the one illustrated in the screenshot below: 
From the edited block:
The ordeer should be: Top toolbar enabled
Basically, navigation through the blocks in Select mode seems broken in this PR. Edit modeWhat Shift+Tab should do from an edited block? Visually, the Document tools toolbar and the Block tools toolbar look like a unique toolbad. However, they are still separated. They are two separated ARIA toolbars.
However, when the Top bar is enabled, does it really make sense to have two separated ARIA toolbars? Seantically, DOcument tools and Block tools have.a different purpose and belong to a different context. However, the real advantage of the ARIA toolbar pattern is to reduce the amount of tab stops. As such, I'd recommend to try to combine Document and Block tools in only one ARIA toolbar:
Things that need more accessibility thoughtsTab/Shift+Tab can navigate between different toolbars,I'm a bit thorn on this. While on one hand it could help, on the other hand it could be very unexpected interaction. I'd like to see this idea considered carefully by the accessibility team cc @alexstine @joedolson Show block tools / Hide block tools buttons in the Top toolbarThese buttons and the underlying functionality are something I'd really like to be reconsidered. I'm not sure hiding UI controls just because there's not enough space and making the reappaer on request is any goof from an usability and accessibility perspective. The root problem is that the UI doesn't provide enough space. Hiding or squeesing content just to make it fit into a limited space is what I'd consider an anti-pattern and generally not a great idea. The root problem should be solved instead. I'd say the current implementation cures the symptom rather than the actual disease. Visual breakage in the Post editorRight now, looks like the Block toolbar in the Top toolbar has some max-widht applied os some other CSS that limits its width and makes only a few button appears. When arrowing through thte buttons, a horizontal scrollbar appears. See animated GIF below:
|
|
I think the Alt+F10 shortcut is a well established pattern in not only editing programs but a good majority of online editors. I disagree that Shift+Tab should move to the options button but I also disagree that Shift+Tab should move to the block toolbar by default. I want Shift+Tab/Tab reserved for in-block actions and if you want to move to other parts of the editor, there is region navigation/landmarks for that. This is a common pattern that Slack and Microsoft Teams alike have started to implement. Keeping the Tab/Shift+Tab keys constrained to content within a region and drawing hard separation for how you move around the application. It takes a little bit for users to learn these new interactions, but it is far more stable in the long term. As far as screen readers switching modes in navigation mode, this is expected and not a bug from this PR. It has happened for the longest time now which is why I'm in favor of stripping this feature out completely. The only way to prevent it would be to wrap it in We currently have the problem of pressing Shift+Tab/Tab to navigate toolbars. I think, now that all 3 toolbars could appear in the same DOM position, we can add instructional nags to let users know how to switch between them. Having all these different toolbars everywhere is not working out and it does not mirror other editing applications where there is one huge toolbar. I would much rather there be just one toolbar but if the comprimise is combining all the toolbars and then giving users some education on how to navigate them, that works as well. |
|
@afercia Thank you for the detailed review and sharing so many high-level thoughts! I appreciate you testing it, and I hope it was clear enough that I know this has plenty of work left on it. I'm still working on the CSS for the top toolbar. I think for this PR, and to establish the direction of the next pieces, we need to focus on: Do we need to preserve Shift + Tab for moving to the toolbar or can we move away from this pattern and rely on alt + f10 to move to the toolbar? If we can reach agreement on what to do with that, then I think it'll be easier to discuss all the other things as follow-up conversations, like if we need to merge the document and block tools into one toolbar, etc. |
jeryj
requested review from
noisysocks,
draganescu,
talldan,
tellthemachines,
adamziel and
kevin940726
as code owners
| @@ -42,7 +42,7 @@ export default function BlockControlsSlot( { group = 'default', ...props } ) { | |||
| return null; | |||
| } | |||
|
|
|||
| const slot = <Slot { ...props } bubblesVirtually fillProps={ fillProps } />; | |||
There was a problem hiding this comment.
We need to validate this approach of removing bubblesVirtually from BlockControl slot fills so we can bubble the escape keypress. If this is fine, then it would also allow introducing priority ordering to Block toolbar items. If not, then we can use one of the other approaches documented in #53779.
There was a problem hiding this comment.
Not valid. It causes an issue with buttons being added to the navigable toolbar after render (such as the apply and cancel buttons when cropping an image) don't get event listeners applied to them.
jeryj
force-pushed
the
update/block-toolbar-dom
branch
from
b4c8663 to
ea7fd74
Compare
|
Size Change: +92 B (0%) Total Size: 1.65 MB
ℹ️ View Unchanged
|
| isCollapsed, | ||
| isFullscreen, | ||
| blockType, | ||
| ] ); |
| }; | ||
| } | ||
|
|
||
| function EmptyBlockInserter( { |
There was a problem hiding this comment.
This needs documentation. What is it for?
| { ...popoverProps } | ||
| > | ||
| <div className="block-editor-block-list__empty-block-inserter"> | ||
| <Inserter |
There was a problem hiding this comment.
Oh, this is the [+] in an empty block?
This is a tricky one. To hide the popover behind the header when it's within the header DOM means we need to have something within the header with a higher z-index for the popover to hide behind. Everything else in the header needs a high z index to be be on top of the header.
Adding position absolute to all instances of __background-style lets it be excluded from all flexbox calculations, which is what we want. If the div is empty, it still gets calculated and pushes elements around.
This test was already covered well by 'ensures base block toolbars use roving tabindex'
This method requires forwarding a ref from the editor level down to the navigable toolbar so that the escape unselect shortcut can be blocked and the navigable toolbar event listener will still fire. Blocking the global escape event shouldn't be necessary, but we have a combination of a few things all combining to create a situation where: - Because the block toolbar uses createPortal to populate the block toolbar fills, we can't rely on the React event bubbling to hit the onKeyDown listener for the block toolbar - Since we can't use the React tree, we use the DOM tree which _should_ handle the event bubbling correctly from a `createPortal` element. - This bubbles via the React tree, which hits this `unselect` escape keypress before the block toolbar DOM event listener has access to it. Also, this is better than attaching it to all of the children in the block toolbar because when new items are attached to the toolbar (such as via the bubblesVirtually slots or the apply/cancel buttons when cropping an image) we can still catch the events. Otherwise, those buttons are added after the mount, and the children don't receive the listener.
…rder of the params matters
jeryj
force-pushed
the
update/block-toolbar-dom
branch
from
8f5a876 to
3a96e02
Compare
|
My thoughts on getting this to a mergeable state:
Follow-up PRs:
The work for the follow-up PRs is basically done as well, so there aren't technical hold-ups to any of them. The only hold-up is agreeing on a route. I think going this route will help us reach mergeable states faster and allow us to have more productive conversations about each topic. |
|
Agreed. 👍 |
| shouldUseKeyboardFocusShortcut = true, | ||
| __experimentalInitialIndex: initialIndex, | ||
| __experimentalOnIndexChange: onIndexChange, | ||
| handleOnKeyDown, |
There was a problem hiding this comment.
Remove this prop -- looks unused.
|
Closing in favor of #55787 |
Fixes #53013 and #45767. Likely others too.
All mouse + visual placements should be the same as in trunk.
This is a big commit with a large potential for bugs.
TODOs:
escapevia entire visual header as well as block and rich text toolbars (for when the editor is loaded without the WP chrome) : Focus loss when closing the block toolbar with the Escape key #45767What?
This PR refactors how the block toolbar structure works to be more like a classic word editor where alt + f10 moves you to all of the block tools. An escape keypress would bring you back to where you were in the editor.
Why?
How?
__experimentalSelectedBlockToolsslot in the header, then render the selected block tools as a fill in that slot.bubblesVirtuallyfrom the BlockControls slot so the NavigableToolbar can handle theescapekeypress to return focus to the editor. Otherwise, due to how React Portals bubble via the React Tree instead of the DOM, theunselectshortcut catches the keypress before theNavigableToolbar. If this isn't a valid approach, then there are two others documented:Potential Follow-ups
I don't want to include these in this PR, as it's already so large, but these are identified issues for potential follow-ups afterwards:
Testing Instructions
Visual and mouse interactions should all be identical to trunk for both Top Toolbar and the default floating block toolbar. Ideally, you don't notice any changes.
Testing Instructions for Keyboard
Combined Toolbar Controls
Focusing the block toolbar
Returning focus to the block
Related PRs